checkout: preserve skip-worktree for virtual filesystem paths#915
Conversation
c882265 to
59ba655
Compare
When 'git checkout <tree> -- <pathspec>' updates the index with entries from the source tree, update_some() creates a new cache entry that unconditionally clears skip-worktree. In a virtual filesystem repo (core.virtualfilesystem is set), this causes checkout_entry() to attempt unlink() on files that exist only as virtual projections with no physical NTFS entry. The unlink fails with ENOENT, producing 'error: unable to unlink old' messages and exit code 255. Fix this in three places: 1. update_some(): Propagate CE_SKIP_WORKTREE from the existing index entry to the replacement entry when core_virtualfilesystem is set. 2. mark_ce_for_checkout_overlay(): Allow skip-worktree entries that have CE_UPDATE (set by update_some for tree entries) to still match the pathspec, so report_path_error() does not reject them. 3. checkout_worktree(): Skip checkout_entry() for entries that have both CE_MATCHED and CE_SKIP_WORKTREE, since the virtual filesystem provider will serve the correct content from the updated projection. The index is updated to the new tree entry's OID while the working tree write is skipped entirely. The virtual filesystem provider re-reads the updated index and serves the correct content on next access. Same approach as the CE_NEW_SKIP_WORKTREE propagation in deleted_entry() (unpack-trees.c, PR microsoft#865) which avoids unnecessary lstats on virtualized paths during branch switches. Assisted-by: Claude Opus 4.6 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
59ba655 to
d40f13b
Compare
dscho
left a comment
There was a problem hiding this comment.
Let me ask my immediate question first: Wouldn't you expect the file file1.txt to be hydrated after git checkout <revision> -- file1.txt? I ask because I would expect that... git checkout is the command I reach to when I want to check out a file, i.e. lay it on disk. And therefore I find it a bit surprising that the added test verifies that the file is missing from disk after it was checked out.
It is quite possible that I am too far removed from actual VFSforGit use cases (I don't use it myself), and that the tested-for behavior is actually the desirable one.
| # Create a second commit with modified content | ||
| git -c core.virtualfilesystem= checkout -b checkout-path-test && | ||
| echo "modified content" >dir1/file1.txt && | ||
| git -c core.virtualfilesystem= add dir1/file1.txt && | ||
| git -c core.virtualfilesystem= commit -m "modify dir1/file1.txt" && |
There was a problem hiding this comment.
It might make sense to simply use a throw-away worktree for that:
# modify file1.txt in a branch
git worktree add -b checkout-path-test temp &&
test_commit -C temp modify modified-contents dir1/file1.txt &&
git worktree remove temp &&
# check out the modified file1.txt
test_when_finished "git checkout HEAD -- dir1/file1.txt" &&
git checkout checkout-path-test -- dir1/file1.txt &&
# file was not actually hydrated
test_path_is_missing dir1/file1.txt &&
# index has the modified version
test_cmp_rev checkout-path-test:dir1/file1.txt :dir1/file1.txtThere was a problem hiding this comment.
I have to point out that I did not test that code, I merely wanted to illustrate the strategy I had in mind: create a worktree where the change is made, then delete that worktree immediately. Then invoke that git checkout <rev> -- <file> command, then validate.
I'm not sure it matters either way if the file is hydrated afterward - the main issue I'm trying to fix is the error that's getting raised trying to delete the non-existent file before that. I think it's more consistent to honor skip-worktree throughout than to honor it for deleting the old file but not for deploying the new file. In the scenario that brought this to my attention, the user did not care whether the file was hydrated or not afterward - they were trying to do a 'partial revert' of files from a pull request ( My intuition is that in most cases, users are either:
I could easily be wrong about user scenarios for this though. |
I trust your judgment, as you sit much closer to VFSforGit users than I do. So I'm fine with this direction. Do you want to play with my suggested change to the test case? If not, I'd be willing to merge this as-is. |
|
I don't see any precedent for using worktrees like that in the test cases and I think the current approach is a bit easier to follow. I'd prefer to keep the test as is. |
dscho
left a comment
There was a problem hiding this comment.
I don't see any precedent for using worktrees like that in the test cases and I think the current approach is a bit easier to follow. I'd prefer to keep the test as is.
Okay, then I'll merge as-is! Thank you!
Problem
git checkout <tree> -- <pathspec>fails witherror: unable to unlink old '<path>': No such file or directoryand exit code 255 when the target file is a virtual ProjFS projection that has not been materialized as a placeholder in a GVFS-mounted repo.Files that have been accessed (and thus have a ProjFS placeholder with a physical NTFS entry) are unaffected —
unlink()works fine for those. The failure is specific to files that exist only in the virtual projection:lstat()succeeds (ProjFS intercepts stat calls for projected files) butunlink()fails because there is no physical NTFS entry to delete.Even without ProjFS, the unfixed code path is wasteful: it clears skip-worktree, writes the file to disk, and triggers ProjFS placeholder creation — all unnecessary because the virtual filesystem provider will serve the correct content once the index is updated.
Root Cause
update_some()inbuiltin/checkout.ccreates a replacement cache entry withcreate_ce_flags(0)which unconditionally clears skip-worktree. This causesmark_ce_for_checkout_overlay()to mark the entry for checkout, andcheckout_entry_ca()attemptsunlink()+write_entry()on a path that may only exist as a virtual projection with no physical file.Fix
Three changes in
builtin/checkout.c, all gated oncore_virtualfilesystemand/orce_skip_worktree():update_some(): PropagateCE_SKIP_WORKTREEfrom the existing index entry to the replacement entry whencore_virtualfilesystemis set. The index is updated with the new tree entry's OID while the virtual filesystem flag is preserved.mark_ce_for_checkout_overlay(): Allow skip-worktree entries that haveCE_UPDATE(set byupdate_somefor changed tree entries) to still match the pathspec, soreport_path_error()does not reject them.checkout_worktree(): Skipcheckout_entry()for entries that have bothCE_MATCHEDandCE_SKIP_WORKTREE. The virtual filesystem provider will serve the correct content from the updated projection on next access.Non-VFS repos and hydrated files (skip-worktree cleared by the VFS hook) are completely unaffected.
Related
Same approach as the
CE_NEW_SKIP_WORKTREEpropagation in #865 which avoids unnecessary lstats on virtualized paths during branch switches. Both bugs stem from skip-worktree being dropped on replacement cache entries, causing unnecessary physical filesystem operations on virtual files.Tests
Two new tests in
t/t1093-virtualfilesystem.sh:checkout path preserves skip-worktree: Creates two commits with different file content, enables VFS mode with 0% hydration, removes the physical file, runs
checkout HEAD~1 -- file, and verifies the index is updated while no file is written to disk.restore --staged after checkout path: Verifies that
restore --staged --ignore-skip-worktree-bitscorrectly restores the index entry back to HEAD after a checkout-path operation with skip-worktree preserved.Verified that test 1 fails without the product code change (checkout writes the file to disk) and passes with it.